fix(kit_creation): update item visibility and value during kit creation#5411
fix(kit_creation): update item visibility and value during kit creation#5411AlexeyKasianenko wants to merge 8 commits intorubyforgood:mainfrom
Conversation
AlexeyKasianenko
force-pushed
the
5275-kit-creation-fix
branch
from
43d771e to
984aea5
Compare
AlexeyKasianenko
changed the title
|
Automatically unassigned after 7 days of inactivity. |
dorner
left a comment
dorner
left a comment
There was a problem hiding this comment.
Thanks for your work, and sorry for the delay - this one slipped under my radar. Please see my comments.
app/services/item_update_service.rb
Outdated
| kit_value_in_cents = item.kit.items.reduce(0) do |sum, i| | ||
| sum + i.value_in_cents.to_i * item.kit.line_items.find_by(item_id: i.id).quantity.to_i | ||
| end |
There was a problem hiding this comment.
You're doing a lot of extra queries this way... I'd recommend instead looping over the line items, so you have access to the quantity, and then looking at the item. E.g.
kit_value_in_cents = item.kit.line_items.includes(:item).reduce(0) do |sum, line_item|
sum + line_item.item.value_in_cents.to_i * line_item.quantity.to_i
end
app/services/kit_create_service.rb
Outdated
| unless item_creation_result.success? | ||
| raise item_creation_result.error | ||
| end | ||
| kit.items.update_all(visible_to_partners: kit.visible_to_partners) |
There was a problem hiding this comment.
This definitely shouldn't happen - the items' visibility shouldn't be affected by a kit's visibility. The ticket was asking for the kit item (i.e. kit.item, not kit.items) to have its visibility updated.
app/services/kit_create_service.rb
Outdated
| raise item_creation_result.error | ||
| end | ||
| kit.items.update_all(visible_to_partners: kit.visible_to_partners) | ||
| kit_value_in_cents = kit.items.reduce(0) do |sum, i| |
There was a problem hiding this comment.
This probably should be moved into a method on Kit since it's used in multiple places.
| end | ||
| let(:request_unit_ids) { [] } | ||
| let(:kit_value_in_cents) do | ||
| kit.line_items.reduce(0) do |sum, li| |
There was a problem hiding this comment.
Please use actual values in the spec rather than calculating them from the factory. It's better to create explicit records with known values and use those values during checks.
| end | ||
| end | ||
| let(:kit_value_in_cents) do | ||
| line_items_attr.sum do |li| |
spec/system/item_system_spec.rb
Outdated
| RSpec.describe "Item management", type: :system do | ||
| let(:organization) { create(:organization) } | ||
| let(:user) { create(:user, organization: organization) } | ||
| let(:user) { create(:organization_admin, organization: organization) } |
There was a problem hiding this comment.
Why was this change necessary?
There was a problem hiding this comment.
| expect(Item.last.value_in_cents).to eq(123_456) | ||
| end | ||
|
|
||
| it "can update an existing item as a user" do |
There was a problem hiding this comment.
Why did this test have to change? The changes happening elsewhere should not have affected existing behavior.
spec/system/item_system_spec.rb
Outdated
|
|
||
| # Consolidated these into one to reduce the setup/teardown | ||
| it "should display items in separate tabs", js: true do | ||
| it "should display items in separate tabs", js: true, driver: :selenium_chrome do |
dorner
left a comment
dorner
left a comment
There was a problem hiding this comment.
Looking better and getting closer!
| sum + i.value_in_cents.to_i * kit.line_items.find_by(item_id: i.id).quantity.to_i | ||
| end | ||
| kit.update!(value_in_cents: kit_value_in_cents) | ||
| kit.item.update(visible_to_partners: kit.visible_to_partners) |
There was a problem hiding this comment.
Should be update! not update. Otherwise it's swallowing any errors you get.
| subject | ||
| kit.reload | ||
| expect(kit.value_in_cents).to eq(kit_value_in_cents) | ||
| expect(kit.value_in_cents).to eq(params[:value_in_cents]) |
There was a problem hiding this comment.
Can we use actual values here instead of referencing code?
| let(:value_in_cents) { value_in_dollars * 100 } | ||
| let!(:items) { create_list(:item, 2, value_in_cents: value_in_cents, organization: organization) } | ||
| let(:quantity_per_kit) { 5 } | ||
| let(:kit_value_in_dollars) { (items.count * quantity_per_kit * value_in_dollars).round(2) } |
There was a problem hiding this comment.
You should know this number at this point - please use it directly.
| find(:css, '#kit_line_items_attributes_0_quantity').set(quantity_per_kit) | ||
| before do | ||
| visit new_kit_path | ||
| fill_in "Name", with: kit_traits[:name] |
There was a problem hiding this comment.
Can we use strings here instead of kit_traits? Same with items[0].name etc. In general I much prefer actual literal values wherever possible.
4 participants
Resolves #5275
Description
Type of change
How Has This Been Tested?
1/ Sign in as org admin.
3/ View a storage location. You're going to need an item that exists in it for the next step.
3/ Make a new kit (Inventory | Kits | New Kit) , giving it that item, and some market value, and visibility unchecked.
4/ For that kit, change its allocation, adding some to the storage location you looked at in step 1 (Modify allocation, choose the storage location, and put a positive # in "Change kit quantity by". Save.)
5) Check the value per item on the corresponding item for the kit: (Inventory | Items, the view the corresponding item) It should correspond to the value you entered for the kit. The visibility should also match.
Screenshots